Add multi-device recording controls - #160
Conversation
1bcb8d1 to
01a9311
Compare
…cording # Conflicts: # Where/WhereCore/Sources/Persistence/SwiftDataStore.swift # Where/WhereCore/Sources/WhereServices.swift # Where/WhereUI/Tests/SwiftDataInspectorWiringTests.swift
…cording # Conflicts: # AGENTS.md
…cording # Conflicts: # Where/WhereCore/Sources/Persistence/StoreRemoteChangeSource.swift
| CloudKit mirroring. Debug uses `.localOnly`; exercise sync with a Release-signed | ||
| build. | ||
|
|
||
| Before shipping a schema change: |
There was a problem hiding this comment.
This should like part of a validation test script or tool that we should perhaps automate in some way.
We recently added support via the Inspector module for booting the app into different modes, maybe we add some sort of iCloud inspect/test mode to that?
…cording # Conflicts: # AGENTS.md
| .padding(.bottom, stylesheet.spacing.xxxLarge) | ||
| } | ||
|
|
||
| private var recordingTitle: LocalizedStringResource { |
There was a problem hiding this comment.
Do we still have (even, opt-in) access to the device name? If so I think we should request it instead of just doing this fallback.
There was a problem hiding this comment.
Additionally, recording the device name alongside the events, even for single event users is useful for audits should they happen; for non-nefarious users, it shows what device the event came from.
There was a problem hiding this comment.
Ah yes it's an entitlement now, yeah seems worth it https://developer.apple.com/documentation/bundleresources/entitlements/com.apple.developer.device-information.user-assigned-device-name
There was a problem hiding this comment.
Update: We'll do this later.
…cording # Conflicts: # Where/TODOs.md # Where/WhereUI/Tests/WhereSessionTrackingTests.swift
Replace the proposed coalescing-worker model with the shipped generation-token and exclusive-controller-lane design. Exercise authorized, denied, repeated, and reversed commands; retain the old race as a negative control; and prove the stale-permission branch is reachable.
I updated the Tracking Reconciliation TLA+ pilot in Verdict: Verified for these model bounds and assumptions. TLC results:
The deterministic Swift guard, The model uses TLC 1.7.4 / TLC2 2.19 with Temurin 21.0.8+9. It assumes a finite command list, eventual permission completion, and eventual completion of an admitted Core transition. It deliberately excludes reset/import lifecycle operations, device removal, persistence failure, cancellation, process termination, GPS samples, and unbounded command streams; those are not covered by this result. The source mapping, properties, configurations, assumptions, and exclusions are documented in |
|
|
||
| /// The throwaway identity used by demo mode. It is intentionally distinct | ||
| /// from test fixtures and never belongs to the real installation sidecar. | ||
| public static let demo = InstallationRecordingContext( |
There was a problem hiding this comment.
Wondering if we should @_spi(Demo) code? Might be worth it
| /// Bridge this main-actor sidecar to the coordinator's async persistence seam without | ||
| /// exposing the adapter's filesystem details to Core. | ||
| public var backupImportRecoveryPersistence: BackupCoordinator.ImportRecoveryPersistence { | ||
| BackupCoordinator.ImportRecoveryPersistence( |
There was a problem hiding this comment.
Yeah this being a struct we "fill in" vs being a protocol remains weird
| /// therefore mirror physical GPS state without observing every unrelated store transaction or | ||
| /// independently re-running the domain reconciliation policy. | ||
| final class RecordingConfigurationBroadcaster: @unchecked Sendable { | ||
| private let lock = NSLock() |
There was a problem hiding this comment.
Is this the actual modern swift way to do this?
| public enum RecordingDeviceKind: String, Codable, Sendable, Hashable { | ||
| case phone | ||
| case tablet | ||
| case other |
There was a problem hiding this comment.
Add an optional associated string to this?
| /// | ||
| /// Recording consent deliberately does not live here; it stays installation-local while | ||
| /// irreversible removal tombstones sync separately. | ||
| public struct RecordingDeviceMetadataChange: Identifiable, Codable, Sendable, Hashable { |
There was a problem hiding this comment.
How does this work when we have more editable fields on a device? Could we change this to some sort of key path based edit instead? Or maybe nickname becomes an enum of the edit kind? Or would we expect to add more similar types (seems bad)?
There was a problem hiding this comment.
(The enum route seems to make the most sense to me, and I think maintains the codable state?)
| return lhs.id.uuidString < rhs.id.uuidString | ||
| } | ||
|
|
||
| /// Stable winner when CloudKit supplies conflicting values for one immutable event id. |
There was a problem hiding this comment.
"for one immutable event id" this bears a lot of weight; is the ID a UUID, a monotomically increasing int, something else?
| _ lhs: RecordingDeviceMetadataChange, | ||
| _ rhs: RecordingDeviceMetadataChange, | ||
| ) -> Bool { | ||
| if lhs.deviceID != rhs.deviceID { |
There was a problem hiding this comment.
I don't think this check should be first? It feels essentially not deterministic
There was a problem hiding this comment.
(Unless this is a true fallback tiebreaker?)
|
|
||
| /// Irreversible, append-only tombstone retiring one installation identity. | ||
| public struct RecordingDeviceRemoval: Identifiable, Codable, Sendable, Hashable { | ||
| public let id: UUID |
There was a problem hiding this comment.
Same comment as similar comments; can we push this to a typed type?
|
|
||
| public func addEvidence(_ evidence: Evidence, blob: Data? = nil) async throws { | ||
| try await store.perform { try await store.write(evidence: evidence, blob: blob) } | ||
| let epochID = try await (store.dataEpoch()).id |
There was a problem hiding this comment.
oh yeah we gotta add a helper for this, its in so many places
| self, | ||
| selector: #selector(eventChanged(_:)), | ||
| name: NSPersistentCloudKitContainer.eventChangedNotification, | ||
| object: nil, |
There was a problem hiding this comment.
Should we be restricting this to a given store or something?
| self, | ||
| selector: #selector(persistentStoreDidChange(_:)), | ||
| name: .NSPersistentStoreRemoteChange, | ||
| object: nil, |
There was a problem hiding this comment.
Similar to above, should we not restrict this to one store?
| /// Dedicated read context and epoch active for one multi-table snapshot. Reads inside the | ||
| /// snapshot use this pair even if CloudKit imports a newer generation mid-block; the block | ||
| /// then fails its end validation rather than returning mixed-generation state. | ||
| private var snapshotContext: ModelContext? |
There was a problem hiding this comment.
Putting a pin in this one for myself, look up what this is
| } | ||
| return writerContext | ||
| } | ||
| if Self.activeSnapshotStores.contains(storeID) { |
There was a problem hiding this comment.
Hmm, this static reference is setting off my spidey sense; how could we model this without a static reference?
| } | ||
|
|
||
| public func dataEpoch() async throws -> WhereDataEpoch { | ||
| if Self.activeTransactionStores.contains(ObjectIdentifier(self)), let writerEpoch { |
There was a problem hiding this comment.
Oh yeah this is in a lot of places huh? I don't like it!
| id: UUID, | ||
| installationID: RecordingDeviceID, | ||
| ) async throws { | ||
| let context = mutationContext() |
There was a problem hiding this comment.
Random, but I have no idea how this actually gets saved given we don't do it inline here
| if $0.registeredAt != $1.registeredAt { | ||
| return $0.registeredAt < $1.registeredAt | ||
| } | ||
| if $0.systemName != $1.systemName { return $0.systemName < $1.systemName } |
There was a problem hiding this comment.
What exactly is systemName?
| } | ||
| } | ||
|
|
||
| public func recordingDeviceMetadataChanges() async throws -> [RecordingDeviceMetadataChange] { |
There was a problem hiding this comment.
When would a consuming framework call this? It's public but it takes no parameters
There was a problem hiding this comment.
I see it returns something but it still feels weird, but I'm likely wrong!
| ) | ||
| let active = existing.filter { Self.belongs($0.epochID, to: epochID) } | ||
| guard !active.isEmpty else { | ||
| context.insert(SDRecordingDeviceMetadataChange(value: change, epochID: epochID)) |
There was a problem hiding this comment.
We should allow / change to initializing this type from RecordingDeviceMetadataChange directly so we dont need to pass through every value at every calcite
| }), | ||
| ) | ||
| for record in samples { | ||
| for record in samples where Self.belongs(record.epochID, to: epochID) { |
There was a problem hiding this comment.
Yeah this feels like a bit of a code smell to me honestly; what if we pushed all these epoch-driven-checks into a sub-object?
| let descriptor = FetchDescriptor<SDDismissedIssue>(predicate: #Predicate { $0.key == key }) | ||
| if let record = try context.fetch(descriptor).first { | ||
| if let record = try context.fetch(descriptor).first(where: { | ||
| Self.belongs($0.epochID, to: epochID) |
There was a problem hiding this comment.
this feels like a patch; if epochs are part of the model now, let's add it to the query vs filtering on it?
There was a problem hiding this comment.
(Again reminder, we've not shipped yet, we can change things!)
|
|
||
| public func trackedRegions() async throws -> Set<Region> { | ||
| let context = readContext() | ||
| let epochID = try readEpochID(in: context) |
There was a problem hiding this comment.
There's gotta be a better way to model this than checking the epoch at every call right?
|
|
||
| public func primaryRegions() async throws -> [PrimaryRegion] { | ||
| let context = readContext() | ||
| let epochID = try readEpochID(in: context) |
There was a problem hiding this comment.
Yeah this is pervasive, there has to be a better way to represent this
|
|
||
| @Model | ||
| final class SDEvidence { | ||
| var epochID: UUID? |
There was a problem hiding this comment.
Again similar comment, should be a typed type
|
|
||
| @Model | ||
| final class SDManualDay { | ||
| var epochID: UUID? |
There was a problem hiding this comment.
Same comment, use a typed epoch id
|
|
||
| @Model | ||
| final class SDDismissedIssue { | ||
| var epochID: UUID? |
There was a problem hiding this comment.
Yep we gotta change this out; each time I see it, feels weird as bare UUID
| /// new epoch before writing their result, so records uploaded later by an offline device remain | ||
| /// in the superseded epoch and cannot repopulate or alter the new account state. | ||
| public struct WhereDataEpochID: RawRepresentable, Codable, Sendable, Hashable { | ||
| public let rawValue: UUID |
1 participant
Summary
Why
A device left at home can record a stationary location that contradicts the user's actual travel. The first implementation solved that with an account-wide, append-only assignment DAG that selected Off or exactly one recorder. That was technically strong, but it made ordinary consent, CloudKit merging, backup restore, transfer, acknowledgement, and conflict UX much more complicated than the product needs.
This version keeps the useful 99%: each installation owns its own recording consent, onboarding makes a conservative recommendation, other devices expose read-only status, and a user can still permanently remove a lost, sold, or retired identity from anywhere. Apple Lost Mode or remote erase remains the security boundary for a stolen device.
Product behavior
Architecture and safety
DeviceRecordingControllerserializes this installation's local choice, physical GPS transition, profile registration, advisory check-in, removal observation, and failed-Off cleanup barrier.RecordingDeviceRemovalis an immutable global tombstone. Unreadable rows invalidate the removal read; identical CloudKit duplicates collapse across epochs, while conflicting same-ID payloads fail closed.WhereDataEpoch.resetBarrieruses the causal epoch graph to identify installations registered before an observed account reset, including concurrent-reset synthetic frontiers.InstallationRecordingContext.RecordingChoicemakes unconfirmed, Off, and On-with-cutoff mutually exclusive persisted states.LocationHistoryReaderis the shared removal-aware boundary used by reports, widgets, recent activity, issue scans, and foreground duplicate checks.LocationOutboxnow stores complete bounded queue snapshots in JournalKit: the newest intact snapshot wins after a torn tail, the previous JSON sidecar migrates once, and destructive clears durably checkpoint empty before deleting raw bytes. Entries remain data-epoch-pinned, so reset, Replace, or cleanup races cannot replay fixes into another generation. If both the primary store and outbox write fail, ingestion retains the sample in memory and revokes recording authority rather than pretending it is durable.Backup and compatibility
Where/Tools/upgrade-backup.rbupgrades shipped v1/v2 archives to v3 without inventing an installation or recording consentValidation
./test --all: 1,774 unit/integration tests passed./swiftformat --lint./xcstrings --lintswift run bumper config .swift run bumper test .(15 architecture tests)swift run bumper lint . --timingsgit diff --checkRollout
Before shipping, deploy the additive CloudKit schema to Production and complete the two-device validation checklist in
Where/Where/README.md. Use./Where/install --cloudkitfor that validation; an ordinary Debug install intentionally remains local-only.